Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add EIP-5976: Handbook for EIP Authors #5976

Closed
wants to merge 18 commits into from
Closed

Conversation

xinbenlv
Copy link
Contributor

When opening a pull request to submit a new EIP, please use the suggested template: https://github.com/ethereum/EIPs/blob/master/eip-template.md

We have a GitHub bot that automatically merges some PRs. It will merge yours immediately if certain criteria are met:

  • The PR edits only existing draft PRs.
  • The build passes.
  • Your GitHub username or email address is listed in the 'author' header of all affected PRs, inside .
  • If matching on email address, the email address is the one publicly listed on your GitHub profile.

@github-actions github-actions bot added c-new Creates a brand new proposal e-number Waiting on EIP Number assignment s-draft This EIP is a Draft labels Nov 15, 2022
@eth-bot
Copy link
Collaborator

eth-bot commented Nov 15, 2022

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):


(fail) .github/workflows/ci.yml

classification
ambiguous
  • '.github/workflows/ci.yml' must be in eip-###.md format; this error will be overwritten upon relevant editor approval

(pass) eip-5976.md

classification
updateEIP
  • passed!

@lightclient
Copy link
Member

This is already in the EIP template?

@xinbenlv
Copy link
Contributor Author

xinbenlv commented Nov 15, 2022

This PR is not ready to review.

My intentions is to create an informational EIP not a template. I will flip to ready for review when ready.

@lightclient
Copy link
Member

It would be better to only open the PR / draft once there is actually some content or explanation.

@xinbenlv xinbenlv closed this Nov 15, 2022
@xinbenlv
Copy link
Contributor Author

Sounds good. Closing for now. Will reopen when ready

@xinbenlv xinbenlv reopened this Nov 16, 2022
@github-actions github-actions bot added t-informational and removed e-number Waiting on EIP Number assignment labels Nov 16, 2022
@xinbenlv xinbenlv marked this pull request as ready for review November 16, 2022 02:29
@xinbenlv
Copy link
Contributor Author

xinbenlv commented Nov 16, 2022

@lightclient I drafted the main piece and reopen this pull request.

The motivation was that I got some good advice from you on #5875, and I want to write it down to help myself to think when authoring new opcode proposals. But in the meanwhile I realize we can use this as a handbook for other suggestions for new authors. Hence this informational EIP.

Some of the suggestions in this draft EIP are solely my personal opinion and might be wrong. Corrections and advices are greatly welcomed!

@xinbenlv xinbenlv changed the title Create a new informational EIP for author instruction Create EIP-5976: Handbook for EIP Authors Nov 16, 2022
@Pandapip1 Pandapip1 changed the title Create EIP-5976: Handbook for EIP Authors Add EIP-5976: Handbook for EIP Authors Nov 16, 2022
@Pandapip1 Pandapip1 marked this pull request as draft November 16, 2022 11:56
@Pandapip1 Pandapip1 marked this pull request as ready for review November 16, 2022 11:56
EIPS/eip-5976.md Outdated Show resolved Hide resolved
EIPS/eip-5976.md Outdated Show resolved Hide resolved
EIPS/eip-5976.md Outdated Show resolved Hide resolved
EIPS/eip-5976.md Outdated
Comment on lines 72 to 74
## Security Considerations

No security issues introduced.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be omitted for informational EIPs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried, EIPW wouldnt let me

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be overwritten.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove that from EIPW side?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it should. CC @SamWilsn.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to remove Security Considerations

xinbenlv and others added 3 commits November 16, 2022 06:56
Co-authored-by: Pandapip1 <[email protected]>
Co-authored-by: Pandapip1 <[email protected]>
Co-authored-by: Pandapip1 <[email protected]>
EIPS/eip-5976.md Outdated Show resolved Hide resolved
EIPS/eip-5976.md Outdated Show resolved Hide resolved
EIPS/eip-5976.md Outdated Show resolved Hide resolved
EIPS/eip-5976.md Outdated Show resolved Hide resolved
EIPS/eip-5976.md Outdated Show resolved Hide resolved
EIPS/eip-5976.md Outdated Show resolved Hide resolved
EIPS/eip-5976.md Outdated
Comment on lines 67 to 69
## Rationale

Help authors better streamline the authoring and reduce review round-trip time.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can also be omitted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is legit rationale and I lean towards keeping the Rationele unless you are against...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am against including a rationale for this EIP, as:

  1. This is actually motivation, not Rationale
  2. The "design decisions" made in this EIP are exceptionally obvious and well-documented in EIP-1.

Copy link
Contributor Author

@xinbenlv xinbenlv Nov 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Removed, respect your opposition.

Pinging @SamWilsn for updating EIPW to exempt "Informational" track from mandating "Rationale " and "Security Considerations"

EIPS/eip-5976.md Show resolved Hide resolved
Copy link
Member

@Pandapip1 Pandapip1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review so far:

EIPS/eip-5976.md Outdated Show resolved Hide resolved
EIPS/eip-5976.md Outdated Show resolved Hide resolved
EIPS/eip-5976.md Outdated Show resolved Hide resolved
EIPS/eip-5976.md Outdated Show resolved Hide resolved
EIPS/eip-5976.md Outdated Show resolved Hide resolved
EIPS/eip-5976.md Outdated Show resolved Hide resolved
@@ -0,0 +1,63 @@
---
Copy link
Contributor Author

@xinbenlv xinbenlv Jan 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rationale and Security Considerations were suggested to be removed by Editor as deemed not applicable to this type of EIP. This will need a manual merge

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should add this to:

unchecked: 1, 5069, 5757

@Pandapip1 Pandapip1 added this to the Manual Merge Queue milestone Jan 10, 2023
@Pandapip1 Pandapip1 self-requested a review February 1, 2023 14:56
@Pandapip1 Pandapip1 removed this from the Manual Merge Queue milestone Feb 1, 2023
@github-actions
Copy link

There has been no activity on this pull request for 2 weeks. It will be closed after 3 months of inactivity. If you would like to move this PR forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

@github-actions github-actions bot added the w-stale Waiting on activity label Feb 16, 2023
@xinbenlv
Copy link
Contributor Author

Still WIP

@github-actions github-actions bot removed the w-stale Waiting on activity label Feb 18, 2023
@github-actions
Copy link

github-actions bot commented Mar 5, 2023

There has been no activity on this pull request for 2 weeks. It will be closed after 3 months of inactivity. If you would like to move this PR forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

@github-actions github-actions bot added the w-stale Waiting on activity label Mar 5, 2023
@github-actions
Copy link

This pull request was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback or request a review in a comment.

@github-actions github-actions bot closed this Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-new Creates a brand new proposal s-draft This EIP is a Draft t-informational w-stale Waiting on activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants